Skip to content

Updated sap_hypervisor_node_preconfigure(redhat_ocpv) #99

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Updated sap_hypervisor_node_preconfigure(redhat_ocpv) #99

wants to merge 15 commits into from
+417 −369

Conversation

newkit
Copy link
Member

@newkit newkit commented Jun 24, 2025 •
edited
Loading

Updated sap_hypervisor_node_preconfigure(redhat_ocpv)

- Added assert for kubeconfig - Unified storageclass to sapstorage for both trident and hpp and make configurable 
- trident: make parameters configurable in role 
- removed unused create-sap-bridge.yml, label-worker-invtsc.yml, sriov-enabled-unsupported-nics.sh 
- restructured tasks - added flags for finegranular tasks control, mainly for debugging 
- add vlan interface support - nmstate: wait for webhooks pods to be available 
- fix bridge and NAD name for additional bridges - added MCP wait to when installing hostpath provisioner 
- Storageclass names and default is configurable now - Wait and check for hco-webhook pod 
- hostpath provisioner: mkfs.xfs in pod, simplified systemd startup scripts

Deprecated vars

    sap_hypervisor_node_preconfigure_ocp_admin_username
    sap_hypervisor_node_preconfigure_ocp_admin_password
    sap_hypervisor_node_preconfigure_ocp_kubeconfig_path
    sap_hypervisor_node_preconfigure_ocp_ca_cert
    sap_hypervisor_node_preconfigure_ocp_extract_kubeconfig
    sap_hypervisor_node_preconfigure_ocp_endpoint
    sap_hypervisor_node_preconfigure_ocp_ca_cert

@newkit newkit self-assigned this Jun 24, 2025
@newkit newkit force-pushed the feature_update_sap_hypervisor_node_preconfigure branch 2 times, most recently from bfacbb9 to 95c547c Compare June 24, 2025 09:46
Updated sap_hypervisor_node_preconfigure(redhat_ocpv)
de09779 
    - sap_hypervisor_node_preconfigure(redhat_ocpv)
        - Added assert for kubeconfig
        - Unified storageclass to sapstorage for both trident and hpp and
          make configurable
        - trident: make parameters configurable in role
        - removed unused create-sap-bridge.yml, label-worker-invtsc.yml, sriov-enabled-unsupported-nics.sh
        - restructured tasks
        - added flags for finegranular tasks control, mainly for debugging
        - add vlan interface support
        - nmstate: wait for webhooks pods to be available
        - fix bridge and NAD name for additional bridges
        - added MCP wait to when installing hostpath provisioner
        - Storageclass names and default is configurable now
        - Wait and check for hco-webhook pod
        - hostpath provisioner: mkfs.xfs in pod, simplified systemd
          startup scripts
@newkit newkit force-pushed the feature_update_sap_hypervisor_node_preconfigure branch from 95c547c to de09779 Compare June 24, 2025 11:42
@newkit newkit requested a review from geetikakay June 26, 2025 13:32
@marcelmamula marcelmamula self-requested a review July 3, 2025 12:47
marcelmamula
marcelmamula requested changes Jul 9, 2025
View reviewed changes
marcelmamula
marcelmamula reviewed Jul 15, 2025
View reviewed changes
.../sap_hypervisor_node_preconfigure/tasks/platform/redhat_ocp_virt/storage/install-trident.yml
annotations:
storageclass.kubernetes.io/is-default-class: "true"
storageclass.kubernetes.io/is-default-class: "{{ 'true' if sap_hypervisor_node_preconfigure_cluster_config.trident.default_storageclass is true else 'false' }}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do not need to check if true as Ansible has default check.
storageclass.kubernetes.io/is-default-class: "{{ 'true' if sap_hypervisor_node_preconfigure_cluster_config.trident.default_storageclass else 'false' }}"

But it also does not make sense here. default_storageclass is bool, so it will always be either true or false. So if your issue is with bool vs string, then maybe just check this approach:
storageclass.kubernetes.io/is-default-class: "{{ sap_hypervisor_node_preconfigure_cluster_config.trident.default_storageclass | d('true') | string }}"

Copy link
Member Author

@newkit newkit Jul 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your suggested code does render the bool true to the string "True". This is not what is expected, we need the string "true". Hence I've reverted your suggestion and stick to my working code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@newkit I dont use these defaults anymore, because of issues like this, so I always use jinja2, where you can properly validate it and sanitize it, like working example below.

    - name: Define required trident keys
      ansible.builtin.debug:
        msg: "{{ __class if __class is defined and __class is boolean else true }}"
      vars:
        __class: "{{ sap_hypervisor_node_preconfigure_cluster_config.trident.default_storageclass }}"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the issue with this code?

storageclass.kubernetes.io/is-default-class: "{{ 'true' if sap_hypervisor_node_preconfigure_cluster_config.trident.default_storageclass is true else 'false' }}"

It might not be overly sophisticated but it does what it should and is easy to read.

@marcelmamula
Copy link
Contributor

@newkit Make sure to check ansible-lint and fix issues reported.

Error: Wrong indentation: expected 4 but found 5
Warning: Jinja2 spacing could be improved: {{ { 'cniVersion': '0.3.1', 'name': __sap_hypervisor_node_preconfigure_register_worker_network.name, 'type': 'cnv-bridge', 'bridge': __sap_hypervisor_node_preconfigure_register_worker_network.name, 'macspoofchk': true } | to_json }} -> {{ {'cniVersion': '0.3.1', 'name': __sap_hypervisor_node_preconfigure_register_worker_network.name, 'type': 'cnv-bridge', 'bridge': __sap_hypervisor_node_preconfigure_register_worker_network.name, 'macspoofchk': true} | to_json }}
You can skip specific rules or tags by adding them to your configuration file:
# .config/ansible-lint.yml
warn_list:  # or 'skip_list' to silence them completely
  - yaml[indentation]  # Violations reported by yamllint.

                Rule Violation Summary                
 count tag               profile rule associated tags 
     1 jinja[spacing]    basic   formatting (warning) 
     1 yaml[indentation] basic   formatting, yaml

@newkit
Copy link
Member Author

newkit commented Jul 16, 2025

Linter issues are fixed and deprecated vars are added to initial commit message.

@newkit
Copy link
Member Author

newkit commented Jul 17, 2025

With the latest code all my CI tests are successful now.

@newkit newkit requested a review from marcelmamula July 17, 2025 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geetikakay geetikakay geetikakay left review comments

@marcelmamula marcelmamula Awaiting requested review from marcelmamula

Requested changes must be addressed to merge this pull request.

Assignees

@newkit newkit

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@newkit @marcelmamula @geetikakay